Initial review - lots of small comments, though little in particular I have strong feelings about. I'm happy to see this query get an overhaul, I'm very happy to see lots of test cases, though there is more query code than I'd like from a readability perspective. It would be nice to have some kind of diagram of how all the flows to fit together to aid understanding.

If you make whichever improvements you feel are most valuable, then I expect to approve this. I do also want to briefly check:

• performance
• real world (MRVA) result changes
• CI

I think I've addressed all the comments. Are you able to run the DCA run? I'm not sure I have access to do so anymore.

I've started a DCA run (though I'm not confident I remember the configuration that will work for CPP - we will see).

On the DCA run we gain 5 results and lose 38. To be honest I'm surprised this query had as many as 38 results given how narrow its purpose is, but there does seem to be a lot of date management code around. The new result set LGTM apart from a couple of FPs involving "magic numbers" 80 and 20, used to convert between various "zero" dates. The lost results are a wide assortment including many of the FP cases your changes cover. So good job!

There is an overall 2% analysis time regression, which appears to be likely caused by a slowdown in this query. It doesn't "explode" on any particular project, so there's no crisis here, but I plan to take a look at what the query's spending that time doing...

I also did a MRVA-100 run with similar results (if a less extreme improvement). There are a couple of FPs where we don't spot that tm_mday is (conditionally) set after updating the year, and another use of the conversion constant 80. If you don't mind making a late alteration to the query, I think adding 80 to the list of conversion constants would improve results a little further - but this will not block my approval.

There is an overall 2% analysis time regression

I had a look at the query locally and I didn't get a consistent signal that anything is really wrong performance-wise. I've started a second DCA run to compare.

On the DCA run we gain 5 results and lose 38. To be honest I'm surprised this query had as many as 38 results given how narrow its purpose is, but there does seem to be a lot of date management code around. The new result set LGTM apart from a couple of FPs involving "magic numbers" 80 and 20, used to convert between various "zero" dates. The lost results are a wide assortment including many of the FP cases your changes cover. So good job!

There is an overall 2% analysis time regression, which appears to be likely caused by a slowdown in this query. It doesn't "explode" on any particular project, so there's no crisis here, but I plan to take a look at what the query's spending that time doing...

I also did a MRVA-100 run with similar results (if a less extreme improvement). There are a couple of FPs where we don't spot that tm_mday is (conditionally) set after updating the year, and another use of the conversion constant 80. If you don't mind making a late alteration to the query, I think adding 80 to the list of conversion constants would improve results a little further - but this will not block my approval.

I could add that, but in the cases you are seeing, what does the magic number 80 correspond to? Happy to add new number filters but I like to document why it is removed, ideally if I can tie it to some specific conversion or some specific datatype.

in the cases you are seeing, what does the magic number 80 correspond to?

It seems to be about converting between "years since 1980" and "years since 1900" as best I can tell. e.g. here. I don't know how common "years since 1980" is but I did see this on another database as well.

The 2% analysis performance regression persisted in the second DCA run. It's not a total blocker but it does suggest something could be wrong - and might have a worse impact on databases we haven't tested. I will try and find time to have another look at what is causing this.

don't know how common "years since 1980" is but I did see this on another database as well.

1980/01/01 is the start of the epoch on e.g. DOS, similar to what 1970/01/01 is on Unix. See also https://en.wikipedia.org/wiki/System_time#Operating_systems.

See also https://en.wikipedia.org/wiki/System_time#Operating_systems.

Thanks, great reference!

don't know how common "years since 1980" is but I did see this on another database as well.

1980/01/01 is the start of the epoch on e.g. DOS, similar to what 1970/01/01 on Unix. See also https://en.wikipedia.org/wiki/System_time#Operating_systems.

Added to the filter set.

@jketema how do we feel about the overall 2% analysis slowdown that merging this change will cause? The query is taking longer locally, it's definitely not quick any more, but I've not seen it anywhere close to timeouts and I've no reason to believe it scales worse than any other dataflow query. I can't really isolate a cause apart from the main dataflow (OperationToYearAssignmentFlow) having a large number of sources. It's not a security query, I believe it is in cpp-security-and-quality.qls, so presumably only a portion of users will run it.

@jketema how do we feel about the overall 2% analysis slowdown that merging this change will cause? The query is taking longer locally, it's definitely not quick any more, but I've not seen it anywhere close to timeouts and I've no reason to believe it scales worse than any other dataflow query. I can't really isolate a cause apart from the main dataflow (OperationToYearAssignmentFlow) having a large number of sources. It's not a security query, I believe it is in cpp-security-and-quality.qls, so presumably only a portion of users will run it.

2% is well within the usual noise ratio for DCA.

2% is well within the usual noise ratio for DCA.

It's definitely a bit more than just noise, but I think we should just accept this, as it seems to resolve a whole bunch of FPs.